[Profile] Add guest profile sign-in flow - #116
conniecliu wants to merge 2 commits into
Conversation
This implements the skip sign-in flow for when a user skips the sign-in prompt. Changes include: - New guest profile screen that appears when a user is not logged in. - Replaced main logo png with xml for visual clarity. - Modified buttons and navigation for new flow and UI.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR adds a guest profile screen with Google sign-in, updates sign-in button modifiers, adds supporting icons, and changes root navigation to select guest content and clear onboarding routes when navigating Home. ChangesGuest profile navigation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant RootNavigationViewModel
participant MainNavigationWrapper
participant GuestProfileScreen
participant LoginViewModel
RootNavigationViewModel->>MainNavigationWrapper: emit session navigation event
MainNavigationWrapper->>GuestProfileScreen: render guest profile
GuestProfileScreen->>LoginViewModel: invoke Google sign-in
Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains in the guest profile and navigation changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 5 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
caleb-bit
left a comment
There was a problem hiding this comment.
In the demo, there is a delay when the profile page is loaded, which I'm guessing is just the components waiting on the async data, but to the user this looks like a weird flicker. Do we have loading state designs for the profile page? If we don't, you can just add that as a Github issue and rerequest review. If we do, I think it's correct to implement it in this PR so the UI displayed is correct.
- Share shimmer placeholders between Home and Profile - Crossfade profile loading, error/retry, and loaded content - Initialize profile as loading to prevent empty-content flashes - Resolve the saved skip-login preference before showing the first screen
Problems
Changes/FixesProfile screen flicker
Returning guest startup flicker
Testing
RecordingsUpdated demo (with the profile screen fix)Demo of restarting the app and re-opening as a guest |
|
|
||
| @Composable | ||
| fun LogInButton(onRequestResult: (Credential) -> Unit) { | ||
| fun LogInButton( |
There was a problem hiding this comment.
Very minor unrelated nit: I think LoginButton is more correct.
| ) | ||
| } | ||
|
|
||
| // TODO: Try to move parts that don't require Context to viewmodel |
There was a problem hiding this comment.
It doesn't have to be in this PR, but this TODO should definitely be done. Business logic inside a component file is pretty unexpected. Feel free to add a Github issue or make a stacked PR for this.
| // Wait for the saved skip-login preference before creating the graph or consuming navigation | ||
| // events. Otherwise a returning guest briefly sees Onboarding before being sent to Home. | ||
| if (!rootNavigationUiState.isStartupReady) { | ||
| Box(Modifier.fillMaxSize().background(Color.White)) |
There was a problem hiding this comment.
Nit: I don't think we should be defaulting to a completely white screen for anything. Can we keep it on the splash screen until it loads? I don't have too much experience with splash screen API, but try using this reference.
| ) { | ||
| data class RootNavigationUiState( | ||
| val isLoggedIn: Boolean = false, | ||
| // The initial destination is temporary until the saved skip preference has been read. |
There was a problem hiding this comment.
I don't think this comment is that useful here.
| ) | ||
| ) { | ||
| // Determines the guest-to-authenticated transition | ||
| internal fun withSession(loggedIn: Boolean, destination: UpliftRootRoute): RootNavigationUiState { |
There was a problem hiding this comment.
Can we rename this? withSession is not too informative.
| Crossfade( | ||
| targetState = contentState, | ||
| modifier = Modifier.fillMaxSize().padding(innerPadding), | ||
| label = "Profile" |
There was a problem hiding this comment.
Nit: extract strings into strings.xml.
| when (state) { | ||
| ProfileContentState.Loading -> ProfileLoading(loadingShimmer) | ||
| // Reuse the existing retry UI so a failed request does not look like an empty profile. | ||
| ProfileContentState.Error -> MainError(reload = onRetry) |
There was a problem hiding this comment.
Nit: here, onRetry is presumably a lambda, whereas reload sounds like it could be a boolean flag. onReload would make this make more sense.
| @Preview(showBackground = true, widthDp = 393, heightDp = 769) | ||
| @Composable | ||
| private fun GuestProfilePreview() { | ||
| GuestProfileScreenContent {} |
There was a problem hiding this comment.
Preview should use theme (UpliftTheme)
Overview
This implements the skip sign-in flow for when a user skips the sign-in prompt. Changes include:
Details on the flow and UI can be found in this Figma.
Changes Made
GuestProfileScreenthat appears only if the user skipped the sign-in prompt during early onboarding.GuestProfileScreenMainNavigationWrapperandRootNavigationViewModelto prevent the user from backing back into the onboarding flow after onboarding is completed + direct them to login if they haven't already + be directed to Home.Test Coverage
Screenrecording
Summary by CodeRabbit